Refactor theme fonts class for readability #661

vcanales · 2024-05-31T12:37:25Z

What?

This Pull Request refactors the them fonts class to improve code readability and maintainability. The main focus is on cleaning up the code.

I felt inclined to do this because when debugging for #660, I found some parts of the code hard to follow.

Why?

This PR addresses the need to make the code more readable and maintainable by:

Adding documentation and comments.
Simplifying functions and improving their readability.

How?

Key Changes:

Class documentation added: Added descriptions and the purpose of the CBT_Theme_Fonts class.
Function improvements:
- Inline Descriptions/Justifications:
  - Refactored make_theme_font_src_absolute() to use array_map for cleaner array handling.
  - Replaced double newlines and improved spacing for better readability.
- Changed foreach to array_map: This reduces the complexity and makes the handling of arrays cleaner.
Code clean-up:
- Removed some redundant comments in favor of "self documenting" practices.
- Used shorthand assignment (?? null) for checking array keys.
- Introduced variables for commonly used expressions.
Function calls:
- Replaced CBT_Theme_Fonts:: with self:: for static method calls within the class, enhancing readability and context awareness.
Added test cases for parts of the code that could break on further refactors.

matiasbenedetto

Probably we should use this refactor to improve the tests of this class. They currently relay on setting users, creating new themes dynamically and after removing it without restoring the previous state.

Example:

create-block-theme/tests/test-theme-fonts.php

Lines 26 to 60 in eabd382

    
           public function test_copy_activated_fonts_to_theme() { 
        
           	wp_set_current_user( self::$admin_id ); 
        
           	$user_data_begin = CBT_Theme_JSON_Resolver::get_user_data()->get_settings(); 
        
           	$test_theme_slug = $this->create_blank_theme(); 
        
           	$this->activate_user_font(); 
        
           	$user_data_before  = CBT_Theme_JSON_Resolver::get_user_data()->get_settings(); 
        
           	$theme_data_before = CBT_Theme_JSON_Resolver::get_theme_data()->get_settings(); 
        
           	$this->save_theme(); 
        
           	$user_data_after  = CBT_Theme_JSON_Resolver::get_user_data()->get_settings(); 
        
           	$theme_data_after = CBT_Theme_JSON_Resolver::get_theme_data()->get_settings(); 
        
           	// ensure that the font was added and then removed from user space 
        
           	$this->assertarraynothaskey( 'typography', $user_data_begin ); 
        
           	$this->assertequals( 'open-sans', $user_data_before['typography']['fontFamilies']['custom'][0]['slug'] ); 
        
           	$this->assertarraynothaskey( 'typography', $user_data_after ); 
        
           	// Ensure that the font was added to the theme 
        
           	$this->assertCount( 1, $theme_data_before['typography']['fontFamilies']['theme'] ); 
        
           	$this->assertCount( 2, $theme_data_after['typography']['fontFamilies']['theme'] ); 
        
           	$this->assertEquals( 'open-sans', $theme_data_after['typography']['fontFamilies']['theme'][1]['slug'] ); 
        
           	// Ensure that the URL was changed to a local file and that it was copied to where it should be 
        
           	$this->assertEquals( 'file:./assets/fonts/open-sans-normal-400.ttf', $theme_data_after['typography']['fontFamilies']['theme'][1]['fontFace'][0]['src'][0] ); 
        
           	$this->assertTrue( file_exists( get_stylesheet_directory() . '/assets/fonts/open-sans-normal-400.ttf' ) ); 
        
           	$this->uninstall_theme( $test_theme_slug ); 
        
           }

I think this kind of test is the cause of the destructive tests we found some time ago and we tried to workaround it here: #618

Could we follow the approach we are using in the base case for the Readme class ?. It can improve 3 aspects of these tests:

Fine tune control and always predictable active theme for test.
No destructive test env.
Avoid realying on CBT complex functions.

matiasbenedetto · 2024-05-31T13:40:34Z

tests/test-theme-fonts.php

+	}
+
+	public function test_array_font_src() {
+		wp_set_current_user( self::$admin_id );


This is probably not needed.

Without it, the tests break:

Time: 00:00.357, Memory: 42.50 MB There was 1 error: 1) Test_Create_Block_Theme_Fonts::test_copy_activated_fonts_to_theme Undefined array key "typography" /var/www/html/wp-content/plugins/create-block-theme/tests/test-theme-fonts.php:43 ERRORS! Tests: 67, Assertions: 163, Errors: 1. Script phpunit handling the test event returned with error code 2 ✖ Command failed with exit code 2 Command failed with exit code 2

Probably we should use this refactor to improve the tests of this class. They currently relay on setting users, creating new themes dynamically and after removing it without restoring the previous state.

I agree that we should work on that, but since the tests currently cover the already-refactored code, I'd prefer refactoring them in another PR.

includes/create-theme/theme-fonts.php

Co-authored-by: Jason Crist <jcrist@pbking.com>

pbking

Seems fine to me. Quality improvements.

matiasbenedetto reviewed May 31, 2024

View reviewed changes

pbking mentioned this pull request Jun 4, 2024

Check if theme fonts present before removing #660

Merged

vcanales added 4 commits June 4, 2024 14:45

refactor theme fonts class for readability

9025c50

add test for src and url handling

cd5d64f

add more assertions to theme deactivation fn

ca60fbd

refactor missing array cast

2b409f8

vcanales force-pushed the try/code-readability-refactor branch from ded5ff5 to 2b409f8 Compare June 4, 2024 13:45

pbking reviewed Jun 4, 2024

View reviewed changes

includes/create-theme/theme-fonts.php Outdated Show resolved Hide resolved

vcanales and others added 2 commits June 4, 2024 11:34

Update includes/create-theme/theme-fonts.php

5fee721

Co-authored-by: Jason Crist <jcrist@pbking.com>

re-add $user_settings

2464bc9

pbking approved these changes Jun 4, 2024

View reviewed changes

add additional assertion

899487c

vcanales merged commit 8d65218 into trunk Jun 4, 2024
2 checks passed

vcanales deleted the try/code-readability-refactor branch June 4, 2024 17:18

MaggieCabrera mentioned this pull request Sep 20, 2024

Add a main tag wrapper around the query loop #726

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor theme fonts class for readability #661

Refactor theme fonts class for readability #661

vcanales commented May 31, 2024

matiasbenedetto left a comment

matiasbenedetto May 31, 2024

vcanales Jun 4, 2024

vcanales Jun 4, 2024

pbking left a comment

	public function test_copy_activated_fonts_to_theme() {

	wp_set_current_user( self::$admin_id );

	$user_data_begin = CBT_Theme_JSON_Resolver::get_user_data()->get_settings();

	$test_theme_slug = $this->create_blank_theme();

	$this->activate_user_font();

	$user_data_before = CBT_Theme_JSON_Resolver::get_user_data()->get_settings();
	$theme_data_before = CBT_Theme_JSON_Resolver::get_theme_data()->get_settings();

	$this->save_theme();

	$user_data_after = CBT_Theme_JSON_Resolver::get_user_data()->get_settings();
	$theme_data_after = CBT_Theme_JSON_Resolver::get_theme_data()->get_settings();

	// ensure that the font was added and then removed from user space
	$this->assertarraynothaskey( 'typography', $user_data_begin );
	$this->assertequals( 'open-sans', $user_data_before['typography']['fontFamilies']['custom'][0]['slug'] );
	$this->assertarraynothaskey( 'typography', $user_data_after );

	// Ensure that the font was added to the theme
	$this->assertCount( 1, $theme_data_before['typography']['fontFamilies']['theme'] );
	$this->assertCount( 2, $theme_data_after['typography']['fontFamilies']['theme'] );
	$this->assertEquals( 'open-sans', $theme_data_after['typography']['fontFamilies']['theme'][1]['slug'] );

	// Ensure that the URL was changed to a local file and that it was copied to where it should be
	$this->assertEquals( 'file:./assets/fonts/open-sans-normal-400.ttf', $theme_data_after['typography']['fontFamilies']['theme'][1]['fontFace'][0]['src'][0] );
	$this->assertTrue( file_exists( get_stylesheet_directory() . '/assets/fonts/open-sans-normal-400.ttf' ) );

	$this->uninstall_theme( $test_theme_slug );

	}

Refactor theme fonts class for readability #661

Refactor theme fonts class for readability #661

Conversation

vcanales commented May 31, 2024

What?

Why?

How?

Key Changes:

matiasbenedetto left a comment

Choose a reason for hiding this comment

matiasbenedetto May 31, 2024

Choose a reason for hiding this comment

vcanales Jun 4, 2024

Choose a reason for hiding this comment

vcanales Jun 4, 2024

Choose a reason for hiding this comment

pbking left a comment

Choose a reason for hiding this comment